-
Notifications
You must be signed in to change notification settings - Fork 73
[LG-5532] feat(time-input): Segment state utils #3385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LG-5532] feat(time-input): Segment state utils #3385
Conversation
…omponents with context integration
…tate handling in TimeInputInputs component
…segment rules and default values
… support 12/24 hour formats
…cale dependency and enhancing formatting logic
… format output with and without seconds
…meInputDisplayContext
…kMode and size, and add console log for debugging in TimeInputInputs
…omponent, covering rendering, value updates, and keyboard interactions
…omponent, covering rendering, value updates, and keyboard interactions
…12HourFormat prop, replacing TimeSegments with TimeSegment for improved clarity and consistency
…ity in time input context
… in TimeInputSegment tests
…db/leafygreen-ui into LG-5538/segments-parse-time
…ygreen-ui into LG-5532/segments-display-values
stephl3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finished reviewing the latter half of the PR, but still reviewing more 🫡
| import { isEverySegmentFilled } from '../isEverySegmentFilled/isEverySegmentFilled'; | ||
| import { isEverySegmentValueExplicit } from '../isEverySegmentValueExplicit/isEverySegmentValueExplicit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import { isEverySegmentFilled } from '../isEverySegmentFilled/isEverySegmentFilled'; | |
| import { isEverySegmentValueExplicit } from '../isEverySegmentValueExplicit/isEverySegmentValueExplicit'; | |
| import { isEverySegmentFilled } from '../isEverySegmentFilled'; | |
| import { isEverySegmentValueExplicit } from '../isEverySegmentValueExplicit'; |
| export const isEverySegmentFilled = (segments: TimeSegmentsState) => { | ||
| const isEverySegmentFilled = Object.values(segments).every(segment => { | ||
| const isSegmentDefined = isDefined(segment); | ||
| const isSegmentEmpty = segment === ''; | ||
| return !isSegmentEmpty && isSegmentDefined; | ||
| }); | ||
| // check if all segments are not empty | ||
| return isEverySegmentFilled; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values of an object that has a type of TimeSegmentsState will always be strings, so this could be simplified to:
const isEverySegmentFilled = Object.values(segments).every(segment => segment !== '');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth considering if it's still necessary to have a util at that point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this for now and reevaulate in the next PR in this chain that uses it
| segment: segment as TimeSegment, | ||
| value: value as string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these type assertions necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I see Object.entries() is expanding the types to be strings. I think it'd make more sense to move the assertion to that line so it propagates to these later lines. something like:
(Object.entries(segments) as Array<[TimeSegment, string]>)
| defaultMin: getDefaultMin({ is12HourFormat })[segment as TimeSegment], | ||
| defaultMax: getDefaultMax({ is12HourFormat })[segment as TimeSegment], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can optimize this by creating the initial objects for defaultMinValues and defaultMaxValues outside of the loop
| segments: TimeSegmentsState; | ||
| is12HourFormat: boolean; | ||
| }): boolean => { | ||
| return Object.entries(segments).every(([segment, value]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar note as above about asserting types when using Object.entries()
| import { TimeSegment, TimeSegmentsState } from '../../shared.types'; | ||
| import { getTimeSegmentRules } from '../getTimeSegmentRules'; | ||
|
|
||
| export const isExplicitSegmentValue = (is12HourFormat: boolean) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this helper sounds like it would return a boolean. Maybe createExplicitTimeSegmentValidator is more appropriate?
| describe('hour', () => { | ||
| describe('returns false', () => { | ||
| test('if hour is 0', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could accept the mild duplication trade-off and flatten this for readability
| describe('hour', () => { | |
| describe('returns false', () => { | |
| test('if hour is 0', () => { | |
| describe('hour segment', () => { | |
| test('returns false when hour is 0', () => { |
| describe('returns true', () => { | ||
| describe('when single digit and value is greater than or equal to the min explicit value (2)', () => { | ||
| test.each(range(2, 10))('%i', i => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly for this:
| describe('returns true', () => { | |
| describe('when single digit and value is greater than or equal to the min explicit value (2)', () => { | |
| test.each(range(2, 10))('%i', i => { | |
| test.each('returns true for single digit hour %i', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't continue repeating feedback but similarly for the later blocks, I think it would help readability to flatten or block these with conditions or labels as opposed to assertion statements
| // If the date is valid and all segments are explicit, then the value should be set. | ||
| const isValidDateAndSegmentsAreExplicit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be me, but comments like this seem redundant because it re-states the variable name in sentence format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
… up imports in shouldSetValue
…ment handling and improving type safety
…TimeSegmentValidator for clarity and update its usage in isEverySegmentValueExplicit
packages/date-utils/src/index.ts
Outdated
| export { isSameTZDay } from './isSameTZDay'; | ||
| export { isSameTZMonth } from './isSameTZMonth'; | ||
| export { isSameUTCDay } from './isSameUTCDay'; | ||
| export { isSameUTCDayAndTime } from './isSameUTCDayAndTime'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't been looking for it for time-input but do changes to date-utils need a changeset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! I was planning on adding all changesets related to the segment integration in a separate PR
| import { isSameUTCDay } from '.'; | ||
|
|
||
| const TZOffset = -5; | ||
| const timeZone = 'America/New_York'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would more explicitly name this constant
| // September 9, 2023 21:00 EDT | ||
| const local = newTZDate({ | ||
| timeZone, | ||
| year: 2023, | ||
| month: 8, | ||
| date: 9, | ||
| hours: 21, | ||
| }); // September 10, 2023 01:00 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while making these changes to these tests, it could be helpful to rewrite the test statements because they're quite confusing currently since they just say "returns true" and "returns false"
| describe('packages/date-utils/isSameUTCMonth', () => { | ||
| beforeEach(() => { | ||
| mockTimeZone('America/New_York', TZOffset); | ||
| mockTimeZone(timeZone, TZOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly, for these tests, it could be beneficial to add context to the test statements that currently only state boolean values
| * @param day2 - The second date to check | ||
| * @returns Whether the two dates are the same day and time in UTC | ||
| */ | ||
| export const isSameUTCDayAndTime = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe isSameUTCDateTime since we're comparing DateType?
| */ | ||
| const { selectUnit, setSelectUnit } = useSelectUnit({ | ||
| dayPeriod: timeParts.dayPeriod, | ||
| dayPeriod: timeParts.dayPeriod as DayPeriod, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand most date time parts being string types since they are input fields, but have you considered making dayPeriod strictly a DayPeriod so you don't have to include this type assertion?
| * @param segments - The segments to check | ||
| * @returns Whether some segment exists | ||
| */ | ||
| export const doesSomeSegmentExist = (segments: TimeSegmentsState) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates isEverySegmentFilled at this point, so we should consolidate
| option => option.displayName === dayPeriod, | ||
| ); | ||
|
|
||
| return selectUnitOption ?? unitOptions[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why it it's possibly not found? feels like there's something else to update to prevent that from being possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.find() return type is T | undefined even though we know that it shouldn't return undefined so we're just adding a fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we update the type of UnitOptions to use DayPeriod for it's object values, it should be safe to assert that a value is always found and simplify this to:
return unitOptions.find(option => option.displayName === dayPeriod)!;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true. In this case, it will always be one of the options.
| dateValues: { | ||
| day: string; | ||
| month: string; | ||
| year: string; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an existing type for this?
| /** | ||
| * Check if all segments are filled and valid (not necessarily explicit). If they are, return the UTC date. | ||
| */ | ||
| if ( | ||
| isEverySegmentFilled(segments) && | ||
| isEverySegmentValid({ segments, is12HourFormat }) | ||
| ) { | ||
| return newTZDate({ | ||
| year: Number(year), | ||
| month: Number(month) - 1, | ||
| date: Number(day), | ||
| hours: Number(converted12hTo24hHour), | ||
| minutes: Number(minute), | ||
| seconds: Number(second), | ||
| timeZone, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Check if any segments are filled. If not, return null. This means all segments are empty. | ||
| */ | ||
| if (!doesSomeSegmentExist(segments)) { | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Return an invalid date object if some segments are empty or invalid. | ||
| */ | ||
| return new Date('invalid'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since isEverySegmentFilled and doesSomeSegmentExist are the same, I think this can be approached differently. Also, I think it will be more readable if some condition leads to an early return of an invalid date, and the final return is return newTZDate(...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're different. isEverySegmentFilled checks that all segments are filled, while doesSomeSegmentExist checks if at least one segment is filled.
We need to check if all segments are empty. If they are, then we return null, so we use doesSomeSegmentExist for this check. It will return true if at least one segment is filled. We can't use isEverySegmentFilled because it will return false if at least one segment is empty. There is a chance that the other segments can be filled.
We also need to check if every segment is filled. That's when we use isEverySegmentFilled. We can't use doesSomeSegmentExist because that will return true as soon as one segment is filled. So there is a chance that the other segments can be empty.
I get confused easily with every and some, so I hope this explanation makes sense. If not, happy to jump on a call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yeah, I see that they use a different method. My b!
…eExplicit tests for clarity and consistency
…ce getNewUTCDateFromSegments logic for better clarity
…and adjust related logic in TimeInputInputs and getFormattedDateTimeParts
…t handling and update getNewUTCDateFromSegments to utilize it
…ec for improved readability and consistency
…proving comments and consistency in date conversions
… by renaming timeZone variable for clarity
…rity across various utility tests
…and isSameUTCMonth.spec for better clarity and consistency
… across components
| option => option.displayName === dayPeriod, | ||
| ); | ||
|
|
||
| return selectUnitOption ?? unitOptions[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we update the type of UnitOptions to use DayPeriod for it's object values, it should be safe to assert that a value is always found and simplify this to:
return unitOptions.find(option => option.displayName === dayPeriod)!;
| [DateTimePartKeys.dayPeriod]: DayPeriod; | ||
| }; | ||
|
|
||
| export type DateParts = Pick<DateTimeParts, 'day' | 'month' | 'year'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating another DateParts type, I was actually wondering if there was an existing one in one of the other date/time-related packages. It looks like there is DateSegment but it's in the date-picker package. It'd be ideal to centralize some types in a shared package like date-utils or input-box. It would keep things more DRY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I like this idea, but I want to think it through a little more on where it should be placed and what should be included. I can create a ticket for this.
| /** | ||
| * Check if all segments are filled and valid (not necessarily explicit). If they are, return the UTC date. | ||
| */ | ||
| if ( | ||
| isEverySegmentFilled(segments) && | ||
| isEverySegmentValid({ segments, is12HourFormat }) | ||
| ) { | ||
| return newTZDate({ | ||
| year: Number(year), | ||
| month: Number(month) - 1, | ||
| date: Number(day), | ||
| hours: Number(converted12hTo24hHour), | ||
| minutes: Number(minute), | ||
| seconds: Number(second), | ||
| timeZone, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Check if any segments are filled. If not, return null. This means all segments are empty. | ||
| */ | ||
| if (!doesSomeSegmentExist(segments)) { | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Return an invalid date object if some segments are empty or invalid. | ||
| */ | ||
| return new Date('invalid'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yeah, I see that they use a different method. My b!
…lidating enums and improving type usage
|
Coverage after merging LG-5532/segments-state-utils into shaneeza/segment-logic-integration will be
Coverage Report for Changed Files
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
753fca9
into
shaneeza/segment-logic-integration
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
This PR introduces utility functions for managing time input segment state, including validation, formatting, and conversion between 12-hour and 24-hour formats. The implementation handles timezone conversions, segment validation, and provides comprehensive test coverage.
This PR is the third PR in a chain of PRs
🧪 How to test changes